[DO NOT MERGE] fix: bypass JVM proxy for Kubernetes API client connections#341
Closed
TheChosenMok wants to merge 3 commits into
Closed
[DO NOT MERGE] fix: bypass JVM proxy for Kubernetes API client connections#341TheChosenMok wants to merge 3 commits into
TheChosenMok wants to merge 3 commits into
Conversation
Signed-off-by: Andre Dietisheim <adietish@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
…partial (crw-11741) Signed-off-by: Andre Dietisheim <adietish@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
OkHttp inherits the JVM default ProxySelector, which in corporate environments routes cluster API calls through an HTTP forward proxy. This may cause silent failures (403, proxy auth errors, PKIX) Set Proxy.NO_PROXY on all ApiClient OkHttp instances. Co-authored-by: Cursor <cursoragent@cursor.com>
TheChosenMok
requested review from
adietish,
azatsarynnyy and
vrubezhny
as code owners
July 21, 2026 21:27
📝 WalkthroughWalkthroughChangesThe PR updates IntelliJ IDEA/Gateway Gradle routing, refactors kubeconfig file and cluster monitoring for concurrency and state-based updates, and configures OpenShift API clients to bypass JVM proxy settings. IDE build and run routing
Kubeconfig file and cluster monitoring
OpenShift API proxy bypass
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant FileWatcher
participant KubeConfigMonitor
participant KubeConfigUtils
FileWatcher->>KubeConfigMonitor: notify file change
KubeConfigMonitor->>KubeConfigUtils: getAllConfigFiles and getClusters
KubeConfigUtils-->>KubeConfigMonitor: cluster list
KubeConfigMonitor-->>KubeConfigMonitor: publish current StateFlow value
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
TheChosenMok
changed the title
TheChosenMok
marked this pull request as draft
July 21, 2026 21:34
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@build.gradle.kts`:
- Around line 244-250: Preserve the runIdeForUiTests task compatibility while
adding runIdeIdea, or update every invocation in
.github/workflows/run-ui-tests.yml atomically to use the replacement task.
Ensure Linux, Windows, and macOS UI-test jobs can still launch the IDE
successfully.
In `@src/main/kotlin/com/redhat/devtools/gateway/kubeconfig/FileWatcher.kt`:
- Around line 80-89: Update addFile and the registerDirectory flow in
FileWatcher so a missing parent directory does not let IOException or
NoSuchFileException escape when tracking a not-yet-existing file. Handle the
failed directory registration consistently with the watcher’s existing
lifecycle, while still retaining the file for later monitoring once its parent
becomes available; preserve normal registration behavior for existing
directories.
In `@src/main/kotlin/com/redhat/devtools/gateway/kubeconfig/KubeConfigMonitor.kt`:
- Around line 94-126: Handle exceptions from kubeConfigUtils.getClusters()
within refreshClusters’ launched coroutine, logging a structured error and
surfacing the refresh failure through the existing user-visible error mechanism.
Keep _clusters unchanged when parsing fails, while preserving mutex handling and
generation checks for successful refreshes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: 72d108e3-cfc8-4a30-8e9e-a3a72205dc65
📒 Files selected for processing (9)
.run/Run Plugin (IDEA).run.xmlbuild.gradle.ktsgradle.propertiessrc/main/kotlin/com/redhat/devtools/gateway/kubeconfig/FileWatcher.ktsrc/main/kotlin/com/redhat/devtools/gateway/kubeconfig/KubeConfigMonitor.ktsrc/main/kotlin/com/redhat/devtools/gateway/openshift/apiclient/BaseClientBuilder.ktsrc/main/kotlin/com/redhat/devtools/gateway/openshift/apiclient/LinkClientBuilder.ktsrc/test/kotlin/com/redhat/devtools/gateway/kubeconfig/FileWatcherTest.ktsrc/test/kotlin/com/redhat/devtools/gateway/kubeconfig/KubeConfigMonitorTest.kt
💤 Files with no reviewable changes (1)
- gradle.properties
Comment on lines
+244
to
+250
| // Visible next to runIde under "intellij platform" in the Gradle tool window | ||
| register("runIdeIdea") { | ||
| type = IntelliJPlatformType.IntellijIdeaUltimate | ||
| version = providers.gradleProperty("platformVersion") | ||
|
|
||
| plugins { | ||
| robotServerPlugin() | ||
| bundledPlugin("com.jetbrains.gateway") |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Preserve or migrate the UI-test Gradle task.
.github/workflows/run-ui-tests.yml still invokes runIdeForUiTests on Linux, Windows, and macOS. Removing that task causes every UI-test job to fail before the IDE starts. Update the workflow atomically or retain a compatibility task.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@build.gradle.kts` around lines 244 - 250, Preserve the runIdeForUiTests task
compatibility while adding runIdeIdea, or update every invocation in
.github/workflows/run-ui-tests.yml atomically to use the replacement task.
Ensure Linux, Windows, and macOS UI-test jobs can still launch the IDE
successfully.
Comment on lines
80
to
89
| fun addFile(path: Path): FileWatcher { | ||
| if (!path.exists() | ||
| || !path.isRegularFile()) { | ||
| return this | ||
| } | ||
| val parentDir = path.parent | ||
| if (parentDir != null | ||
| && !monitoredFiles.contains(path)) { | ||
| val watchKey = parentDir.register(watchService, | ||
| StandardWatchEventKinds.ENTRY_CREATE, | ||
| StandardWatchEventKinds.ENTRY_MODIFY, | ||
| StandardWatchEventKinds.ENTRY_DELETE | ||
| ) | ||
| registeredKeys[watchKey] = parentDir | ||
| registerDirectory(parentDir) | ||
| monitoredFiles.add(path) | ||
| onFileChanged?.invoke(path) | ||
| invokeOnFileChanged(path) | ||
| } | ||
| return this | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Unhandled IOException when a monitored file's parent directory doesn't exist yet.
addFile() was changed to support tracking non-existent files (per the docstring), but registerDirectory() still calls directory.register(...) unconditionally. If the parent directory itself doesn't exist (a plausible case for a custom KUBECONFIG entry or a not-yet-mounted config dir — exactly the scenario this feature targets), Path.register() throws IOException/NoSuchFileException, which propagates uncaught through addFile() → KubeConfigMonitor.startWatchingNew()/updateMonitoredPaths()/start(), which per the linked snippet in DevSpacesServerStepView has no surrounding try/catch.
🛡️ Proposed fix
private fun registerDirectory(directory: Path) {
if (registeredDirectories.values.any { it == directory }) {
return
}
- val watchKey = directory.register(
- watchService,
- StandardWatchEventKinds.ENTRY_CREATE,
- StandardWatchEventKinds.ENTRY_MODIFY,
- StandardWatchEventKinds.ENTRY_DELETE
- )
- registeredDirectories[watchKey] = directory
+ try {
+ val watchKey = directory.register(
+ watchService,
+ StandardWatchEventKinds.ENTRY_CREATE,
+ StandardWatchEventKinds.ENTRY_MODIFY,
+ StandardWatchEventKinds.ENTRY_DELETE
+ )
+ registeredDirectories[watchKey] = directory
+ } catch (e: IOException) {
+ // Parent directory doesn't exist yet; the file stays in monitoredFiles
+ // but won't be watched until a later addFile()/updateMonitoredPaths() retries.
+ }
}Also applies to: 116-127
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/kotlin/com/redhat/devtools/gateway/kubeconfig/FileWatcher.kt` around
lines 80 - 89, Update addFile and the registerDirectory flow in FileWatcher so a
missing parent directory does not let IOException or NoSuchFileException escape
when tracking a not-yet-existing file. Handle the failed directory registration
consistently with the watcher’s existing lifecycle, while still retaining the
file for later monitoring once its parent becomes available; preserve normal
registration behavior for existing directories.
Comment on lines
+94
to
126
| /** | ||
| * Schedules a cluster refresh. Multiple rapid calls (e.g. per-file [FileWatcher.addFile] | ||
| * notifies during bootstrap) coalesce: only the latest generation parses and emits. | ||
| * Paths are snapshotted when the work runs, not when it is scheduled, so a burst of | ||
| * sync registrations yields one full-list emit. | ||
| * | ||
| * [MutableStateFlow] skips collector notifications when the new list equals the previous. | ||
| */ | ||
| internal fun refreshClusters() { | ||
| logger.info("Reparsing kubeconfig files. Monitored paths: $monitoredPaths") | ||
| val allClusters = kubeConfigUtils.getClusters(monitoredPaths.toList()) | ||
| val gen = refreshGeneration.incrementAndGet() | ||
| scope.launch { | ||
| logger.info("Emitting ${allClusters.size} clusters to SharedFlow") | ||
| _clusters.emit(allClusters) | ||
| refreshMutex.withLock { | ||
| if (gen != refreshGeneration.get()) { | ||
| return@withLock | ||
| } | ||
| val monitored = fileWatcher.getMonitoredFiles().toList() | ||
| logger.info("Reparsing kubeconfig files. Monitored paths: $monitored") | ||
| val allClusters = kubeConfigUtils.getClusters(monitored) | ||
| if (gen != refreshGeneration.get()) { | ||
| return@withLock | ||
| } | ||
| val previous = _clusters.value | ||
| _clusters.value = allClusters | ||
| if (previous == allClusters) { | ||
| logger.info("Skipping notify; clusters unchanged (${allClusters.size})") | ||
| } else { | ||
| logger.info( | ||
| "Updated clusters (${allClusters.size}): " + | ||
| allClusters.map { "${it.name}@${it.url}" } | ||
| ) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Unhandled parsing errors from kubeConfigUtils.getClusters().
The call reads/parses external kubeconfig files but isn't guarded. An exception (malformed YAML, permission error, etc.) propagates out of the launched coroutine uncaught — the mutex is released fine, but the refresh silently fails with no structured error handling or user-visible feedback, and _clusters is left stale without any signal that a refresh failed.
🛡️ Proposed fix
val monitored = fileWatcher.getMonitoredFiles().toList()
logger.info("Reparsing kubeconfig files. Monitored paths: $monitored")
- val allClusters = kubeConfigUtils.getClusters(monitored)
+ val allClusters = try {
+ kubeConfigUtils.getClusters(monitored)
+ } catch (e: Exception) {
+ logger.warn("Failed to parse kubeconfig files: $monitored", e)
+ return@withLock
+ }
if (gen != refreshGeneration.get()) {
return@withLock
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| /** | |
| * Schedules a cluster refresh. Multiple rapid calls (e.g. per-file [FileWatcher.addFile] | |
| * notifies during bootstrap) coalesce: only the latest generation parses and emits. | |
| * Paths are snapshotted when the work runs, not when it is scheduled, so a burst of | |
| * sync registrations yields one full-list emit. | |
| * | |
| * [MutableStateFlow] skips collector notifications when the new list equals the previous. | |
| */ | |
| internal fun refreshClusters() { | |
| logger.info("Reparsing kubeconfig files. Monitored paths: $monitoredPaths") | |
| val allClusters = kubeConfigUtils.getClusters(monitoredPaths.toList()) | |
| val gen = refreshGeneration.incrementAndGet() | |
| scope.launch { | |
| logger.info("Emitting ${allClusters.size} clusters to SharedFlow") | |
| _clusters.emit(allClusters) | |
| refreshMutex.withLock { | |
| if (gen != refreshGeneration.get()) { | |
| return@withLock | |
| } | |
| val monitored = fileWatcher.getMonitoredFiles().toList() | |
| logger.info("Reparsing kubeconfig files. Monitored paths: $monitored") | |
| val allClusters = kubeConfigUtils.getClusters(monitored) | |
| if (gen != refreshGeneration.get()) { | |
| return@withLock | |
| } | |
| val previous = _clusters.value | |
| _clusters.value = allClusters | |
| if (previous == allClusters) { | |
| logger.info("Skipping notify; clusters unchanged (${allClusters.size})") | |
| } else { | |
| logger.info( | |
| "Updated clusters (${allClusters.size}): " + | |
| allClusters.map { "${it.name}@${it.url}" } | |
| ) | |
| } | |
| } | |
| } | |
| /** | |
| * Schedules a cluster refresh. Multiple rapid calls (e.g. per-file [FileWatcher.addFile] | |
| * notifies during bootstrap) coalesce: only the latest generation parses and emits. | |
| * Paths are snapshotted when the work runs, not when it is scheduled, so a burst of | |
| * sync registrations yields one full-list emit. | |
| * | |
| * [MutableStateFlow] skips collector notifications when the new list equals the previous. | |
| */ | |
| internal fun refreshClusters() { | |
| val gen = refreshGeneration.incrementAndGet() | |
| scope.launch { | |
| refreshMutex.withLock { | |
| if (gen != refreshGeneration.get()) { | |
| return@withLock | |
| } | |
| val monitored = fileWatcher.getMonitoredFiles().toList() | |
| logger.info("Reparsing kubeconfig files. Monitored paths: $monitored") | |
| val allClusters = try { | |
| kubeConfigUtils.getClusters(monitored) | |
| } catch (e: Exception) { | |
| logger.warn("Failed to parse kubeconfig files: $monitored", e) | |
| return@withLock | |
| } | |
| if (gen != refreshGeneration.get()) { | |
| return@withLock | |
| } | |
| val previous = _clusters.value | |
| _clusters.value = allClusters | |
| if (previous == allClusters) { | |
| logger.info("Skipping notify; clusters unchanged (${allClusters.size})") | |
| } else { | |
| logger.info( | |
| "Updated clusters (${allClusters.size}): " + | |
| allClusters.map { "${it.name}@${it.url}" } | |
| ) | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/kotlin/com/redhat/devtools/gateway/kubeconfig/KubeConfigMonitor.kt`
around lines 94 - 126, Handle exceptions from kubeConfigUtils.getClusters()
within refreshClusters’ launched coroutine, logging a structured error and
surfacing the refresh failure through the existing user-visible error mechanism.
Keep _clusters unchanged when parsing fails, while preserving mutex handling and
generation checks for successful refreshes.
adietish
reviewed
Jul 22, 2026
adietish
left a comment
adietish
left a comment
Collaborator
There was a problem hiding this comment.
I dont understand why in this PR (that Cursor created) the code is actually bypassing the proxy rather than set it up as configured in the IntellIJ settings
| */ | ||
| protected fun createHttpClient(sslContext: SSLContext, trustManager: X509TrustManager): OkHttpClient = | ||
| OkHttpClient.Builder() | ||
| .proxy(Proxy.NO_PROXY) |
Collaborator
There was a problem hiding this comment.
odd, why would it bypass the proxy? Dont we want to the exact opposite which is to set up the proxy that is possibly configured in the Gateway/JetBrains IDEA settings
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Proxy.NO_PROXYon allApiClientOkHttp instances inBaseClientBuilderandLinkClientBuilderProxySelector, which in corporate environments routes cluster API calls through an HTTP forward proxy. This may cause silent failures (403, proxy auth errors, PKIX)Note: This branch is based on PR #340 (crw-11741). Merge #340 first, then this.
Test plan
Made with Cursor